-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Attempted to fix #141972 Parameter ABI inconsistency error in Rust for RISC-V #141979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@YingkaiLi-VM please rebase on the current master branch |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #142220) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
@bors @workingjubilee Okay,Merge conflicts resolved. Thanks for your help! |
@rustbot review |
@@ -7,6 +7,9 @@ | |||
//@ normalize-stderr: "(valid_range): [1-9]\.\.=(429496729[0-9]|1844674407370955161[0-9])" -> "$1: $$NON_NULL" | |||
// Some attributes are only computed for release builds: | |||
//@ compile-flags: -O | |||
//@ revisions: generic riscv64 | |||
//@ [riscv64] only-riscv64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this a cross-compiling test?
See this for an example of what that looks like: 6cea550
You will also want to add something like this:
//@ add-core-stubs
#![feature(no_core)]
#![no_core]
extern crate minicore;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If using this approach:
#![feature(no_core)]
#![no_core]
extern crate minicore;
The code will fail to compile with the error: the trait bound str: Sized is not satisfied.
If using this approach (conditional compilation comments):
//@ revisions: generic riscv64
//@ [riscv64] compile-flags: --target riscv64gc-unknown-linux-gnu
//@ [riscv64] needs-llvm-components: riscv
This test will only apply to the target riscv64gc-unknown-linux-gnu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@workingjubilee I'm not sure if my understanding is correct, but personally, I believe this particular test itself is intended to be a generic test. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test can only be blessed if it is cross-compiled. Changes to the specifics of rustc's internals will make this test fail and then, because we do not run tests on riscv targets, we will not run this revision of this test, and it will fall out of sync.
You need //@ add-core-stubs
to make the minicore crate compiled with it.
And "str is not Sized" is already a part of the test output:
rust/tests/ui/abi/debug.riscv64.stderr
Lines 878 to 885 in 3a036a1
error[E0277]: the size for values of type `str` cannot be known at compilation time | |
--> $DIR/debug.rs:57:46 | |
| | |
LL | type TestAbiEqNonsense = (fn((str, str)), fn((str, str))); | |
| ^^^^^^^^^^ doesn't have a size known at compile-time | |
| | |
= help: the trait `Sized` is not implemented for `str` | |
= note: only the last element of a tuple may have a dynamically sized type |
That is already an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy to accept the PR you are currently offering but I need you to understand that you are signing yourself up for this test instead potentially breaking in a new way every time you update rustc and run the tests on your RV64 host. As opposed to now, where it will just be broken in this same way every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for your help. I'll look into how to fix this
No description provided.